Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra coverage testing for char.jl #12882

Merged
merged 1 commit into from
Sep 1, 2015

Conversation

ScottPJones
Copy link
Contributor

Add tests for getindex, bswap, ndims, size and typemin
Note: I noticed a number of inconsistencies that should probably be dealt with in a post-0.4 PR.
getindex('c',1,1,1) is allowed, and returns 'c', but getindex("c",1,1,1) gets an error.
bswap on a Char should probably not be allowed, the operation only makes sense on the underlying codeunit, i.e. UInt32, not on Char.

@ScottPJones ScottPJones force-pushed the spj/testchar branch 2 times, most recently from 516d48f to 519b05f Compare August 31, 2015 13:38
@kshyatt kshyatt added the test This change adds or pertains to unit tests label Aug 31, 2015
@stevengj
Copy link
Member

stevengj commented Sep 1, 2015

Test failure due to #11553.

@ScottPJones
Copy link
Contributor Author

Yes, it's killed basically every PR of coverage tests I've submitted this weekend, sometimes more than once.

# This gets a deprecation warning that seems incorrect
# getindex('a',1.0,1) -> WARNING: indexing with non Integer Reals is deprecated
# Capture spurious deprecation warning
let
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please remove this. You can leave a note to update when the implementation changes, but for now just test what is currently implemented. This is not going to change for 0.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of 0.4, implemented in line 22 of char.jl:

getindex(c::Char, I::Real...) = getindex(c, to_indexes(I...)...)

@jakebolewski
Copy link
Member

Please squash these commits.

Handle deprecation warnings treated as errors

Remove getindex check
@ScottPJones
Copy link
Contributor Author

@jakebolewski Squashed, and I removed the test of the getindex/checkbounds that depend on the deprecated to_index(x::Real). I'm also making a PR to directly deprecate those, because it makes absolutely no sense to deprecate an internal not exported function, without deprecating the functions that use it. I have another PR, #12877 that adds testing, have you seen that one? Thanks.

@ScottPJones
Copy link
Contributor Author

Bump, these have passed the now OOM free tests (yeah!), should be all set.

jakebolewski added a commit that referenced this pull request Sep 1, 2015
Add extra coverage testing for char.jl
@jakebolewski jakebolewski merged commit 0f7dee9 into JuliaLang:master Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants